-
Notifications
You must be signed in to change notification settings - Fork 585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework Fake.Core.Target package #1664
Rework Fake.Core.Target package #1664
Conversation
Apparently this needs a merge/rebase after the latest PR merges, sorry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice - thanks for helping out with this.
|
||
## Migration Guide | ||
|
||
Upgrading to FAKE 5 is a multi step process and has various manual steps in between. Here are the steps: | ||
Upgrading to FAKE 5 is a multi step process and has various manual steps in between. **If you do these steps out of order it will be a lot harder for you to migrate the script successfully**. Here are the steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
getTargetDict().Values | ||
|> Seq.iter (fun target -> | ||
Trace.tracefn " - %s %s" target.Name (match target.Description with Some d -> " - " + d | _ -> "") | ||
Trace.tracefn " Depends on: %A" target.Dependencies) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch.
- Consistent naming between namespace & NuGet pagckage - Introduce FAKE003 code for functions made unaccessible - Review all documentation referencing Target
954d16d
to
b474d43
Compare
Rebase done (someone already fixed doc around Target) |
Any chance to have a look at this PR @matthid ? Thanks |
Yes I'm kind of slow at the moment. next step is to release all the stuff I've merged and update to netcoreapp20 and fixing the release pipeline (some things are not yet automated - see fake.api.github pr). Than I can test the release pipeline for example with this pr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -57,4 +57,4 @@ Target "Default" DoNothing | |||
==> "Default" | |||
|
|||
// start build | |||
RunParameterTargetOrDefault "target" "Default" | |||
RunTargetOrDefault "Default" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Target.RunParameterOrDefault
or Target.RunOrDefault
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I like your suggestion...I will do it and normalize casing in a new PR, do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I was wrong on this. I have no idea where this file is used and if updating this breaks anything...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking you were saying that RunParameterOrDefault would be a better name than just RunOrDefault?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No your change is fine. I'm not sure why we even have runparameterordefault
- FAKE0001 for moving part from one Module to another | ||
- FAKE0002 for removed API we don't know who is using it and how => please open an issue if you use it | ||
- FAKE0003 for API that is no more accessible (basically became internal) => please open an issue if you use it | ||
- FAKE0004 for API not yet migrated, waiting for your contribution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice idea.
@@ -53,27 +53,27 @@ Now we have the following options: | |||
## Final targets | |||
|
|||
Final targets can be used for TearDown functionality. | |||
These targets will be executed even if the build fails but have to be activated via ActivateFinalTarget(). | |||
These targets will be executed even if the build fails but have to be activated via ActivateFinal(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we write Target.ActivateFinal
here to advertise the new usage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
|
||
BuildFailureTarget "ReportErrorViaMail" (fun _ -> | ||
CreateBuildFailure "ReportErrorViaMail" (fun _ -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Target.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
// Activate BuildFailureTarget somewhere during build | ||
ActivateBuildFailureTarget "ReportErrorViaMail" | ||
// Activate Build Failure Target somewhere during build | ||
ActivateBuildFailure "ReportErrorViaMail" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Target.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
|
||
|
||
"Clean" | ||
==> "Build" | ||
==> "Deploy" | ||
|
||
RunTargetOrDefault "Deploy" | ||
Target.RunOrDefault "Deploy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- Fix all the (obsolete) warnings in your build-script to use the new API (see the 'Use the new FAKE-API' section). | ||
- Update to legacy FAKE 5. This should not be breaking. If it breaks you please open an issue. | ||
|
||
- With Paket: add `prerelease` after `nuget FAKE` in paket.dependencies file then `.paket/paket.exe update FAKE`, check that paket.lock references FAKE version 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to remember to remove prerelease
after releasing FAKE.
**/!\ There is a breaking change with NuGet package rename to be consistent between Nuget package and namespace